Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove FinalForm dependency in main form [DHIS2-18373] #425

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

tomzemp
Copy link
Member

@tomzemp tomzemp commented Nov 25, 2024

This addresses performance issues associated with Final Form by removing the FinalForm dependency from the main form. See the original ticket for more information about where performance problems occured: DHIS2-18373

Instead of using form/field state, the field inputs have been refactored

  • to keep values as state variables
  • to set a state variable valueSynced: true when the cell's current state value matches the last synced value
  • active state

To simplify things, I've also used useHighlightedField in some places where we had looked for active field. This could be updated, but apart from the difference in border color of the cell, I wasn't sure if the differentiating between active/highlighted was so relevant. (It does slightly change the calculation of indicators/totals, but I think this is pretty minor and some of our active state management seemed a bit buggy on some input types)

@dhis2-bot
Copy link
Contributor

dhis2-bot commented Nov 25, 2024

🚀 Deployed on https://pr-425--dhis2-data-entry.netlify.app

@dhis2-bot dhis2-bot temporarily deployed to netlify November 25, 2024 14:28 Inactive
@tomzemp tomzemp requested a review from Birkbjo November 25, 2024 14:35
Copy link
Member

@Birkbjo Birkbjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thanks a lot for getting into this so quickly!

src/data-workspace/custom-form/replace-input-node.js Outdated Show resolved Hide resolved
src/data-workspace/data-entry-cell/data-entry-field.js Outdated Show resolved Hide resolved
const [lastSyncedValue, setLastSyncedValue] = useState(() =>
convertBooleanValue(initialValue)
)
const [syncTouched, setSyncTouched] = useState(false)
Copy link
Member

@Birkbjo Birkbjo Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In "genric-input" we just do setValueSynced(value === newValue). What is the reason that this and other fields need the effect and syncTouched? Could we not calculate this, or set the synced value in the onSuccess-callback? Or should the generic-input also use this way of doing it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's more correct to do the side effect with useEffect. In the generic input, which is the first one I wrote, this setValueSynced(value === newValue) was working, but in the other components, it does not work, I suppose because the code is executing before the new value has actually been set.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I just in general try to not use useEffect, and always try to look for other solutions. It adds additional re-renders and complexity. But maybe in this case it's warranted - because we're actually syncing states here.

But I do feel like the need for "syncTouched", "lastSyncedValue", and "setValueSynced" is somewhat complex.
Ideally this is just something we could calculate from mutation state, and potentially the query-cache. But it's hard to argue for such changes when this seems to work great.

src/shared/use-blurred-field.js Outdated Show resolved Hide resolved
@dhis2-bot dhis2-bot temporarily deployed to netlify December 3, 2024 14:35 Inactive
@tomzemp tomzemp marked this pull request as ready for review December 4, 2024 12:03
@dhis2-bot dhis2-bot temporarily deployed to netlify December 6, 2024 15:28 Inactive
@tomzemp tomzemp requested a review from Birkbjo December 9, 2024 08:34
)

const initialValuesInStore =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the logic to check that the values in the zustand store are for the given data form, so as to prevent rendering the input cells before the initial values are present

Copy link
Member

@Birkbjo Birkbjo Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a small (but scary) bug that I think is related to this, but also the service worker somehow.

If you edit a cell - blur and let it save - then refresh. The value will not be shown in the form.
You can also test this with another tab of the same form: if you update the form in one tab, then refresh in another - you won't get the updated data unless you refresh twice.

It seems like the forms initial values are populated with the "offline"-cached version of the values. So I think we need some additional checks for the loading in data-workspace?

We may need to check for "isFetchedAfterMount" on the initialDataValuesFetch query when checking for loading indicator - unless you're offline?

eg:

    if (initialDataValuesFetch.isFetching || (!initialDataValuesFetch.isFetchedAfterMount && !initialDataValuesFetch.isPaused)) {
        return (
            <CenteredContent>
                <CircularLoader />
            </CenteredContent>
        )
    }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😱 thanks for that!

I'm too be honest a little confused with the react-query nomenclature here. I would have thought that since we had refetchOnMount: false for this query that isFetchedAfterMount would remain false, but I guess it's 'isFetchedAfterMountand maybe I'm imaging a variableisRefetchedAfterMount`?

@dhis2-bot dhis2-bot temporarily deployed to netlify December 12, 2024 09:16 Inactive
useValueStore((state) => state.getInitialDataValues())?.formKey ===
validFormKey

useLayoutEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it matters too much here, but I think normal useEffect should suffice.

if (syncTouched) {
setValueSynced(value === lastSyncedValue)
}
}, [value, lastSyncedValue, syncTouched])
Copy link
Member

@Birkbjo Birkbjo Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix exhaustive deps?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️ sorry for that. I'm impressed(?) with myself for managing to miss multiple lint warnings both locally and in github 😬

Copy link
Member

@Birkbjo Birkbjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for fixing this! This is a lot of work and very impressive!

I've mainly a few comments about lint-issues and a bug that needs to be fixed.

I do feel like some of the sync logic could be hoisted to "DataEntryField". Eg. we could have a lastSyncedValue-state there. Then we could update that value, instead of a "synced"-boolean. And compare value (from the store) with lastSyncedValue. Right now we're comparing the state in the field, but it's the data in the dataValueStore that is actually "the source of truth", right? And in theory these could diverge?
But it might be that would complicate things - because I do see some logic to compare the values for eg. the file-field. So I won't block this PR because of this.

@dhis2-bot dhis2-bot temporarily deployed to netlify December 20, 2024 12:03 Inactive
@tomzemp
Copy link
Member Author

tomzemp commented Dec 20, 2024

Thanks for the review (x2)! I've addressed the lint + bug issue 🙏

I do feel like some of the sync logic could be hoisted to "DataEntryField". Eg. we could have a lastSyncedValue-state there. Then we could update that value, instead of a "synced"-boolean. And compare value (from the store) with lastSyncedValue. Right now we're comparing the state in the field, but it's the data in the dataValueStore that is actually "the source of truth", right? And in theory these could diverge?

I think of this PR as a bug fix for the immediate issue of the forms being non-performant because of the react-final-form dependency (which we probably went with for convenience/familiarity/fact that it's called a data entry form even though it's not really a typical form), but it would be good to make a follow-up ticket (feature) to revisit some of the other logic to see if we can improve it further/leverage some of react-query's functionality for checking synced state.

I do wonder though on the specific issue of whether it's more ideal to indicate sync status based on comparison with the input state or the dataValueStore. I would actually think that insofar as the sync state is being recorded to provide a visual cue in the cell, we'd probably want it to reflect what the user is seeing in the cell? Like if for some reason cell shows 1, dataValueStore has 2, and the value that was sent to the server is 1, we'd actually prefer to show that as synced? But this is a point I think it would be good to discuss / revisit the logic. I guess if you went offline you might trigger a resync based on the dataValueStore, but if you're online it seems you'll need to enter a value in the cell to get a POST to update the data value on the server? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants